feat(abtest): hypothesis metadata for pre-registration (Change Set A) - #46
Conversation
Change Set A — hypothesis pre-registration for A/B tests. New domain module (hypothesis.ts): - ExpectedLift: relative (10% lift) or absolute (2pp / currency). - ExperimentOwner: stable org handle + optional display name. - ExperimentScope: channel, experimentFamilyKey, attribution/exclusion windows. Family key validates as [a-z0-9._-]+. - HypothesisMetadata: objective, hypothesis, primaryMetric, expectedLift, owner, experimentScope, createdAt, lockedAt?, checksum?. - validateHypothesisMetadata: strict mode for launch (all required), non-strict for draft (optional fields). - computeHypothesisChecksum: canonical SHA-256 excluding lockedAt/checksum. - lockHypothesis: computes checksum + sets lockedAt; rejects double-lock. - verifyHypothesisChecksum: detects post-lock tampering. AbTest type extensions: - hypothesis?: HypothesisMetadata - assignmentProvenance?: 'manifest_v1' | 'legacy_unavailable' operations.ts abTestSchema: assignmentProvenance optional enum. persistence.ts validator: assignmentProvenance value check. Package entrypoint exports all hypothesis types and functions. Graph anchor for hypothesis tests (197 paths). 19 direct-import tests cover validation (strict/draft), checksum determinism/tampering, locking, and experimentFamilyKey format.
|
@codex please review this hypothesis pre-registration module (Change Set A). Focus areas: HypothesisMetadata type design, checksum canonicalization (excludes lockedAt/checksum), validation (strict launch vs draft), experimentFamilyKey format, and lockHypothesis double-lock prevention. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds checksum-locked hypothesis metadata with strict validation, persistence checks, and assignment provenance. It also adds recipient-domain stratification with constrained quota matrices for deterministic holdout provisioning. ChangesA/B test hypothesis support
Recipient-domain stratification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CreateCommand
participant AbTestService
participant ListmonkIntegration
participant computeStratifiedQuotas
participant Persistence
CreateCommand->>AbTestService: create A/B test with hypothesis
AbTestService->>AbTestService: lock hypothesis and assign provenance
AbTestService->>ListmonkIntegration: provision deterministic holdout
ListmonkIntegration->>computeStratifiedQuotas: audience strata and exact group counts
computeStratifiedQuotas-->>ListmonkIntegration: quota matrix
ListmonkIntegration-->>AbTestService: assignment and stratification result
AbTestService->>Persistence: store hypothesis, provenance, and stratification
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/abtest/src/hypothesis.ts`:
- Around line 168-178: The checksum serialization in the canonical hash flow
must preserve nested values instead of applying the top-level
Object.keys(canonical).sort() replacer recursively. Update the canonical
serialization used by the hypothesis checksum function to recursively sort keys
or explicitly canonicalize nested fields such as primaryMetric, expectedLift,
owner, and experimentScope, then hash that complete stable JSON; add tampering
tests covering each nested field.
- Around line 77-158: Update validateHypothesisMetadata to require createdAt in
strict mode and validate it as a valid ISO timestamp before lockHypothesis can
hash metadata. Add validation for primaryMetric’s declared discriminant and
expectedLift.kind and absolute unit, rejecting values outside the contract while
preserving existing field validation.
In `@packages/abtest/src/operations.ts`:
- Around line 160-162: The operations output schema in
packages/abtest/src/operations.ts around lines 160-162 must include an optional
hypothesis schema matching the draft/locked AbTest shape so MCP results retain
it. In packages/abtest/src/persistence.ts around lines 237-241, update
isStoredAbTest to validate persisted hypothesis metadata, including checksum and
lockedAt integrity whenever present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83628323-c0aa-42cf-bb81-96ca82ccda9e
📒 Files selected for processing (8)
.sampo/changesets/abtest-hypothesis.mdpackages/abtest/src/hypothesis.tspackages/abtest/src/index.tspackages/abtest/src/operations.tspackages/abtest/src/persistence.tspackages/abtest/src/types.tspackages/abtest/tests/hypothesis.test.tsscripts/check-graph-architecture.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99e59e9900
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| experimentScope: metadata.experimentScope, | ||
| createdAt: metadata.createdAt, | ||
| }; | ||
| const json = JSON.stringify(canonical, Object.keys(canonical).sort()); |
There was a problem hiding this comment.
Preserve nested fields in the hypothesis checksum
JSON.stringify applies this array replacer recursively, so nested keys such as type, direction, kind, value, id, and experimentFamilyKey are omitted and each nested object is serialized as {}. Consequently, changing the primary metric, expected lift, owner, or experiment scope after locking still makes verifyHypothesisChecksum return true, defeating the pre-registration integrity guarantee; use deterministic recursive canonicalization that retains nested fields.
Useful? React with 👍 / 👎.
| if (metadata.expectedLift !== undefined) { | ||
| if (!Number.isFinite( | ||
| metadata.expectedLift.value, | ||
| ) || metadata.expectedLift.value <= 0) { | ||
| throw new HypothesisValidationError( | ||
| `expectedLift.value must be finite and positive, received ${metadata.expectedLift.value}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Reject malformed nested launch metadata
Strict validation only requires the nested objects to be present and checks expectedLift.value; it never validates primaryMetric.type/direction, expectedLift.kind or the required absolute-lift unit, and it does not require or validate createdAt. Thus runtime input such as { primaryMetric: {}, expectedLift: { kind: "bogus", value: 1 } } without createdAt passes strict validation and can be locked as a purportedly valid pre-registration.
Useful? React with 👍 / 👎.
| assignmentProvenance: z | ||
| .enum(["manifest_v1", "legacy_unavailable"]) | ||
| .optional(), |
There was a problem hiding this comment.
Include hypothesis in the operation output schema
The AbTest model now includes hypothesis, but this schema extension only adds assignmentProvenance. When a persisted test contains hypothesis metadata, every CLI/MCP invoker passes the serialized test through parseOperationOutput; Zod strips the undeclared hypothesis property, so callers cannot retrieve the pre-registration even though it remains in storage. Add the hypothesis shape to this shared output schema.
AGENTS.md reference: AGENTS.md:L155-L157
Useful? React with 👍 / 👎.
| /** Per-test minimum sample size for the fixed-horizon gate. */ | ||
| minimumTestSampleSize?: number; | ||
| /** Hypothesis metadata for pre-registration (Change Set A). */ | ||
| hypothesis?: import("./hypothesis").HypothesisMetadata; |
There was a problem hiding this comment.
Validate persisted hypothesis records
Adding hypothesis to the persisted AbTest shape without extending isStoredAbTest means validateStoredAbTestStore and loadStoredAbTests accept arbitrary malformed hypothesis objects and post-lock checksum mismatches, after which parseAbTestStore casts them to HypothesisMetadata. Validate the nested shape and locked-state invariants before hydrating file-backed records so downstream launch/report code does not receive untrusted metadata.
AGENTS.md reference: AGENTS.md:L180-L183
Useful? React with 👍 / 👎.
| lockedAt: string = new Date().toISOString(), | ||
| ): HypothesisMetadata { | ||
| if (metadata.lockedAt) { |
There was a problem hiding this comment.
Validate the supplied lock timestamp before locking
When a caller uses the supported timestamp override with lockHypothesis(metadata, ""), the function returns a checksum with an empty lockedAt; verifyHypothesisChecksum then returns false and a subsequent call is allowed because the double-lock guard tests timestamp truthiness. Any other non-ISO string is also accepted as a valid lock timestamp, so validate the override before creating the locked object.
Useful? React with 👍 / 👎.
| primaryMetric: { | ||
| type: "click_rate" | "conversion_rate" | "revenue_per_recipient"; | ||
| direction: "maximize" | "minimize"; | ||
| }; | ||
| expectedLift: ExpectedLift; |
There was a problem hiding this comment.
Couple absolute-lift units to the primary metric
The independent types permit semantically incompatible metadata such as primaryMetric.type: "click_rate" with an absolute currency_per_recipient lift, or revenue_per_recipient with percentage_point; both combinations also pass validation and can be locked. Model or validate the metric/unit pairing so the pre-registered lift has an interpretable meaning for later analysis and reporting.
Useful? React with 👍 / 👎.
| if (!scope.experimentFamilyKey.match(/^[a-z0-9._-]+$/)) { | ||
| throw new HypothesisValidationError( | ||
| `experimentFamilyKey must match [a-z0-9._-]+, received "${scope.experimentFamilyKey}"`, | ||
| ); |
There was a problem hiding this comment.
Require non-empty family-key segments
The character-class check accepts delimiter-only and empty-segment keys such as ., -, .foo, foo., and foo..bar. These do not form the documented dotted family identifier and allow a separator typo to create a different collision namespace rather than being rejected; require an alphanumeric segment at each end and between separators.
Useful? React with 👍 / 👎.
Change Set B of the advanced experimentation followup. Introduces a stratification module that classifies subscribers by email-domain provider and solves a constrained quota matrix so each stratum gets a proportional share of every variant/holdout group. - normalizeDomain / classifyStratum / DEFAULT_STRATIFICATION_POLICY for recipient_domain_provider classification (gmail, naver, daum, kakao, with unknown/other fallbacks). - computeStratifiedQuotas uses largest-remainder per stratum row, then a paired-swap column correction that preserves row sums while matching exact group column counts. Each swap decreases a surplus-group cell and increases a deficit-group cell in the same row, choosing rows by cell deviation from ideal. Verified against 5000 randomized multi-stratum, multi-group trials for row sums, column sums, and non-negativity. - Export the module from packages/abtest/src/index.ts. - Add a graph architecture anchor connecting the stratification tests to the quota solver.
OpenCodeReview findings on the stratification commit: - high: totalAudience was used as the proportional divisor but never validated against the strata/groups sums. A stale or zero value produced silently skewed (or NaN) ideals. Add an explicit equality guard with a descriptive message. - medium: the Phase 2 paired-swap loop could exit early without resolving every column deficit. Add a post-loop assertion that every residual deficit is zero so an unconverged matrix fails loudly. - low: cellDeviation linearly scanned the cells array inside a nested loop. Precompute an idealLookup map and read ideals from it.
Addresses @codex and CodeRabbit review findings on Change Set A. P1: - computeHypothesisChecksum now recursively canonicalizes nested fields. The previous flat Object.keys().sort() array replacer was applied recursively by JSON.stringify, which dropped nested keys and serialized primaryMetric/expectedLift/owner/experimentScope as "{}". Tampering with any nested field after locking no longer passes verification. - validateHypothesisMetadata in strict mode now requires createdAt as a valid ISO 8601 timestamp, validates primaryMetric.type/direction against their enums, and validates expectedLift.kind plus the absolute-lift unit. P2: - Add the hypothesis shape to the shared abTest operation output schema so CLI/MCP callers can retrieve persisted pre-registration metadata instead of having Zod strip it. - Extend isStoredAbTest with isStoredHypothesis so loadStoredAbTests rejects malformed nested hypothesis records and locked-without-checksum states before hydration. - lockHypothesis validates the supplied lockedAt override as ISO 8601, rejecting empty or malformed timestamps that would produce an unverifiable lock. - Couple absolute-lift units to the primary metric: revenue_per_recipient requires currency_per_recipient, and click/conversion_rate require percentage_point. Relative lift stays unit-agnostic. - Tighten experimentFamilyKey validation to require non-empty alphanumeric segments joined by single [._-] separators, rejecting ".", "foo.", "foo..bar", and delimiter-only keys. Tests cover nested-field checksum tampering, every new strict-mode guard, the metric/unit pairing matrix, the family-key segment rules, the lockedAt override validation, and malformed persisted hypothesis rejection.
OpenCodeReview findings on the hardening commit: - medium: isStoredHypothesis only checked that checksum was a 64-char string, but a tampered locked record (valid format, wrong hash) would still hydrate. Now re-verifies the checksum cryptographically via verifyHypothesisChecksum so post-lock tampering is rejected at load. - medium: isStoredHypothesis validated experimentFamilyKey only as a string, weaker than the runtime segment rules. Mirror the /^[a-z0-9]+(?:[._-][a-z0-9]+)*$/ regex so load-time validation matches creation-time validation. - low: lockHypothesis validated the lockedAt override before the metadata, surfacing the less relevant timestamp error first. Reorder to validate metadata first, then the timestamp override. Skipped the 'remove the rawKind cast' suggestion: the discriminated union narrows an invalid kind to never, so the runtime guard cannot compile without the cast. Kept the cast to preserve the defensive runtime check. Tests cover a properly locked hypothesis round-trip, post-lock tampering rejection, and malformed family-key rejection at load time.
|
Addressed all @codex and CodeRabbit review findings: P1 (checksum + strict validation):
P2 (schema, persistence, lock timestamp, metric/unit coupling, family-key):
Verified against 5000 randomized stratification trials, plus 234 abtest unit tests and full |
|
@codex review — the hypothesis hardening and stratification Change Sets (A+B) now have follow-up commits |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60ef214075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** Hypothesis metadata for pre-registration (Change Set A). */ | ||
| hypothesis?: import("./hypothesis").HypothesisMetadata; |
There was a problem hiding this comment.
Wire hypothesis metadata into test creation
When a test is created through the shared CLI/MCP operation, createAbTestInputSchema, CreateAbTestInput, and AbTestConfig provide no hypothesis field, and AbTestService.createTest provisions the assignment manifest without calling lockHypothesis. Consequently, users of either supported surface cannot pre-register this newly exposed metadata, despite it being returned in operation output; add the hypothesis to the shared creation contract and lock it before segmentation creates the manifest.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
| // Pre-registration hypothesis: optional, but the nested shape and the | ||
| // locked-state checksum invariant are validated when present so that | ||
| // loadStoredAbTests never hydrates malformed or tampered metadata. | ||
| (value.hypothesis === undefined || isStoredHypothesis(value.hypothesis)) |
There was a problem hiding this comment.
Require hypotheses to be locked when a manifest exists
When loading a record that contains both assignmentManifest and an unlocked hypothesis, these independent optional checks accept it because isStoredHypothesis requires a checksum only if lockedAt is already present. Such a record can therefore acquire or modify hypothesis content after recipient assignment and still pass persistence validation, defeating the stated pre-registration guarantee; enforce that a hypothesis accompanying a manifest has a valid lock and checksum.
Useful? React with 👍 / 👎.
| export function computeStratifiedQuotas(params: { | ||
| stratumSizes: Record<string, number>; | ||
| groupExactCounts: Record<string, number>; | ||
| groupOrder: string[]; | ||
| totalAudience: number; | ||
| }): StratificationResult { |
There was a problem hiding this comment.
Apply stratified quotas in the assignment path
The new solver is exported but never called by any production code: the holdout provisioning path still invokes segmentSubscribersForHoldout directly, and repository-wide references to computeStratifiedQuotas are limited to this module, its export, tests, and the graph check. Thus A/B tests continue using the existing unstratified manifest regardless of recipient domains, so the released “stratification for A/B test assignment” feature has no operational effect.
Useful? React with 👍 / 👎.
| for (const sk of stratumKeys) { | ||
| const row = quotas[sk]; | ||
| if (!row) continue; | ||
| const surplusQuota = row[surplusGroup] ?? 0; | ||
| if (surplusQuota <= 0) continue; |
There was a problem hiding this comment.
Preserve floor and ceiling bounds during quota correction
When row-wise rounding leaves a column imbalance, accepting any donor with quota > 0 allows repeated swaps to move cells outside the promised floor-or-ceiling allocation. For example, strata {s0:116,s1:105,s2:74,s3:47} and group counts {g0:37,g1:216,g2:63,g3:26} produce s1/g3 = 9 for an ideal of 7.982... (ceiling 8). This can overrepresent a provider stratum even though row and column totals pass; restrict candidate swaps so the donor remains at least its floor and the receiver remains at most its ceiling.
Useful? React with 👍 / 👎.
| if (metadata.createdAt !== undefined) { | ||
| if ( | ||
| typeof metadata.createdAt !== "string" || | ||
| Number.isNaN(Date.parse(metadata.createdAt)) | ||
| ) { | ||
| throw new HypothesisValidationError( | ||
| `createdAt must be a valid ISO 8601 timestamp, received ${JSON.stringify(metadata.createdAt)}`, |
There was a problem hiding this comment.
Validate hypothesis timestamps as ISO 8601
When callers supply audit timestamps, Date.parse accepts non-ISO and normalized-invalid values even though the API promises ISO 8601; for example, createdAt: "0", lockedAt: "01/02/03", and createdAt: "2026-02-30" all pass, with the last silently rolling into March. These ambiguous values are then included in the checksum and accepted by persistence, weakening the pre-registration audit record; use a strict ISO parser such as the datetime schema already used by operation timestamps for both fields.
Useful? React with 👍 / 👎.
| "conversion_rate", | ||
| "revenue_per_recipient", | ||
| ]; | ||
| if (!validTypes.includes(pm.type)) { |
There was a problem hiding this comment.
Reject malformed nested metadata with validation errors
When this published runtime validator receives metadata from JavaScript or parsed JSON, a present but malformed nested value bypasses the undefined check and is dereferenced directly: primaryMetric: null, expectedLift: null, or experimentScope: null throws a raw TypeError, and owner: {id: 123} does the same at .trim(). Validate that each nested value is an object and each field has the expected primitive type before accessing it so invalid user data consistently produces HypothesisValidationError rather than crashing the caller.
Useful? React with 👍 / 👎.
| npm/@listmonk-ops/abtest: minor (Added) | ||
| --- | ||
|
|
||
| Add hypothesis metadata for A/B test pre-registration: structured objective, primary metric, expected lift, owner, and experiment scope with canonical checksum locking. AbTest gains optional hypothesis and assignmentProvenance fields. |
There was a problem hiding this comment.
Document the new public experimentation APIs bilingually
The changesets publish hypothesis locking and recipient-domain stratification as minor user-facing additions, but neither README.md nor README_ko.md documents their contracts, validation rules, or usage, and the package README is unchanged as well. Add matching English and Korean guidance so operators and library consumers can discover and correctly use the newly exported behavior.
AGENTS.md reference: AGENTS.md:L233-L237
Useful? React with 👍 / 👎.
| assignmentProvenance: z | ||
| .enum(["manifest_v1", "legacy_unavailable"]) | ||
| .optional(), |
There was a problem hiding this comment.
Populate assignment provenance for persisted tests
The new assignmentProvenance field is parsed and serialized but never assigned anywhere in production: deterministic holdout provisioning sets assignmentManifest without setting manifest_v1, while full-split and hydrated legacy tests never receive legacy_unavailable. Consequently, every normal CLI/MCP response omits the field, so consumers cannot use it for its stated purpose of distinguishing deterministic manifests from legacy assignments; derive it when provisioning and when hydrating older records.
Useful? React with 👍 / 👎.
| for (const [provider, domains] of Object.entries(policy.providerDomainMap)) { | ||
| if (domains.includes(domain)) { | ||
| return provider; |
There was a problem hiding this comment.
Normalize configured domains before classifying strata
When an operator supplies a custom providerDomainMap containing a semantically valid mixed-case or trailing-dot domain, only the subscriber email is normalized, while domains.includes(domain) compares the configured value verbatim. For example, {google: ["GMAIL.COM"]} classifies user@gmail.com as other, silently skewing the provider quotas; normalize policy domains with the same lowercase, trim, and trailing-dot rules before comparison.
Useful? React with 👍 / 👎.
Addresses the second @codex review's P1/P2 findings on PR #46. P1: - Hypothesis is now wired into creation: CreateAbTestInput/AbTestConfig gain an optional hypothesis field, basic.ts maps snake_case input to the domain type, and AbTestService.createTest locks it (if unlocked) before any provisioning so the assignment manifest is bound to a frozen checksummed hypothesis. - Persistence now requires a hypothesis to be present and locked whenever an assignmentManifest exists, enforcing the pre-registration guarantee that hypothesis content cannot change after recipient assignment. - Stratification is now called in production: segmentSubscribersForHoldout computes the recipient-domain quota matrix from the resolved audience (when a stratification policy is enabled and emails are available) and stores it on AbTest.stratification for reporting/validation. - Document the hypothesis and stratification APIs bilingually (EN/KO) in the package README, covering contracts, validation rules, and usage. P2: - Tighten createdAt/lockedAt to strict ISO 8601: reject values Date.parse silently accepts ("0", "01/02/03", overflowed "2026-02-30"). - Guard nested metadata access so null/non-object primaryMetric/expectedLift/ owner/experimentScope and non-primitive fields raise HypothesisValidationError instead of raw TypeError. - Populate assignmentProvenance during provisioning: manifest_v1 for holdout, legacy_unavailable for full-split. - Preserve floor/ceiling bounds during quota correction by preferring swaps that keep donor >= floor(ideal) and receiver <= ceil(ideal), with a fallback to minimize deviation when column totals otherwise forbid a bounded swap. - Normalize configured provider domains before classification so mixed-case or trailing-dot entries match subscriber domains correctly. Verified against 8000 randomized stratification trials and 240 abtest tests; full bun run check (198 architecture paths) and build pass.
|
Addressed all 9 findings from the second @codex review: P1:
P2:
|
OpenCodeReview findings on the wiring commit: - high: stratification was inside the try/catch that calls deleteListsBestEffort, so a quota invariant failure would cascade into deleting all provisioned lists and tearing down the test. Wrap the computation in its own try/catch so a failure degrades gracefully to an undefined stratification. - medium: emailsAvailable used .some(), so a single member with email classified all members — email-less subscribers were silently bucketed as "unknown". Require every member to carry an email before computing the matrix. - low: combined the email-availability check and classification into a single pass over resolvedMembers.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
packages/abtest/src/stratification.ts (3)
233-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cellDeviationis dead code.The swap loop computes deviations inline (lines 279-281) and never calls this helper. Remove it to avoid confusion about which deviation definition drives the correction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/src/stratification.ts` around lines 233 - 238, Remove the unused cellDeviation helper from the stratification logic. Keep the swap loop’s inline deviation calculation unchanged, since it is the active definition used for correction.
143-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating that counts are non-negative integers.
Sums are checked for agreement, but a negative or fractional
stratumSizes/groupExactCountsentry passes all three invariants and then silently produces fractional ideals and negative quotas that the convergence check cannot detect. A cheap up-frontNumber.isInteger(n) && n >= 0guard per entry would fail fast instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/src/stratification.ts` around lines 143 - 162, Add an upfront validation in the stratification flow before calculating totals, ensuring every value in stratumSizes and groupExactCounts is an integer greater than or equal to zero. Fail fast with an error when any entry violates this constraint, then preserve the existing invariant checks for valid counts.
95-109: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRebuilding the provider lookup on every call makes classification O(members × configured domains).
classifyStratumis called once per audience member inpackages/abtest/src/listmonk-integration.ts(lines 340-346), sobuildProviderLookupre-normalizes the wholeproviderDomainMapfor each subscriber. Expose a prepared classifier (or memoize per policy object) so the lookup is built once per stratification run.♻️ Suggested shape
+const lookupCache = new WeakMap<StratificationPolicyV1, Map<string, string>>(); + +function providerLookupFor(policy: StratificationPolicyV1): Map<string, string> { + let lookup = lookupCache.get(policy); + if (lookup === undefined) { + lookup = buildProviderLookup(policy); + lookupCache.set(policy, lookup); + } + return lookup; +} + export function classifyStratum( email: string, policy: StratificationPolicyV1, ): string { const domain = normalizeDomain(email); if (domain === "") { return policy.unknownStratumKey; } - const lookup = buildProviderLookup(policy); + const lookup = providerLookupFor(policy); const provider = lookup.get(domain);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/src/stratification.ts` around lines 95 - 109, Update classifyStratum and its callers so buildProviderLookup is executed once per stratification run rather than once per audience member. Expose a prepared classifier or equivalent lookup-based API, construct it before the member iteration in listmonk integration, and preserve the existing unknown, provider, and other stratum results.packages/abtest/tests/stratification.test.ts (2)
85-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTautological expectation.
groupKey === "variant:A" ? 500 : 500always yields 500; simplify.- expect(colSum).toBe(groupKey === "variant:A" ? 500 : 500); + expect(colSum).toBe(500);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/tests/stratification.test.ts` around lines 85 - 91, In the stratification test’s quota-sum assertion, simplify the tautological conditional in the loop over groupKey to assert the constant expected total directly, preserving the existing 500 expectation for both variants.
200-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment and assertion disagree.
The comment says each cell stays within 1 of its ideal, but the bound asserted is 1.5. Align the comment with the actual tolerance (or tighten the bound if 1 is the real contract).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/tests/stratification.test.ts` around lines 200 - 203, Align the assertion and comment in the stratification test: either change the comment to document the existing 1.5 tolerance or tighten the toBeLessThanOrEqual bound to 1 if that is the intended contract. Keep the test’s stated behavior and enforced threshold consistent.packages/abtest/src/operations.ts (2)
163-204: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOutput hypothesis schema is looser than the persistence guard.
createdAt/lockedAtare plainz.string()(other timestamps in this schema use.datetime()), andexperimentFamilyKeyskips the segment regex enforced both increateAbTestInputSchema(Line 353) and inisStoredHypothesisinpackages/abtest/src/persistence.ts. Aligning them keeps the three contracts from drifting.♻️ Proposed tightening
experimentScope: z.object({ channel: z.literal("email"), - experimentFamilyKey: z.string(), + experimentFamilyKey: z + .string() + .regex(/^[a-z0-9]+(?:[._-][a-z0-9]+)*$/), attributionWindowHours: z.number().finite().positive(), exclusionWindowHours: z.number().finite().nonnegative(), }), - createdAt: z.string(), - lockedAt: z.string().optional(), + createdAt: z.string().datetime(), + lockedAt: z.string().datetime().optional(), checksum: z.string().optional(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/src/operations.ts` around lines 163 - 204, tighten the output hypothesis schema in the operation output definition: validate createdAt and optional lockedAt with the same datetime constraint used by the surrounding schemas, and apply the established experimentFamilyKey segment pattern used by createAbTestInputSchema and isStoredHypothesis. Keep the existing optional hypothesis shape and all other field validation unchanged.
322-358: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHypothesis input passes the public boundary unvalidated for metric/unit coupling. Both the zod input schema and the command-level mapper accept e.g.
revenue_per_recipient+percentage_point; the violation only surfaces later fromlockHypothesisinsideAbTestService.createTest, after Listmonk subscriber-count calls, as aHypothesisValidationErrorrather than an input validation error.
packages/abtest/src/operations.ts#L322-L358: add a.superRefine/.checkon thehypothesisobject enforcingrevenue_per_recipient → currency_per_recipientandclick_rate|conversion_rate → percentage_pointfor absolute lifts.packages/abtest/src/basic.ts#L51-L85: validate the mapped metadata invalidate()(e.g.validateHypothesisMetadata(mapped, true)wrapped asValidationError) so bad hypotheses fail fast and consistently with other input errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/src/operations.ts` around lines 322 - 358, Update the hypothesis schema in packages/abtest/src/operations.ts (lines 322-358) with a superRefine/check that requires absolute revenue_per_recipient lifts to use currency_per_recipient and absolute click_rate or conversion_rate lifts to use percentage_point. In packages/abtest/src/basic.ts (lines 51-85), validate the mapped hypothesis metadata in validate() via validateHypothesisMetadata(mapped, true), converting failures to ValidationError so invalid input is rejected before service calls.packages/abtest/tests/persistence.test.ts (1)
246-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
assignmentManifest↔ hypothesis clause.None of these cases exercises
isStoredAbTest's manifest gate (packages/abtest/src/persistence.tsLines 251-252). A test that loads a legacy record with anassignmentManifestand no hypothesis would have caught the backward-compatibility break flagged there, and one with an unlocked hypothesis + manifest would pin the intended "must be locked" rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/tests/persistence.test.ts` around lines 246 - 380, Extend the “rejects malformed persisted hypothesis metadata” test to cover the isStoredAbTest assignmentManifest gate: verify a legacy record with assignmentManifest but no hypothesis still loads successfully, and verify a record with assignmentManifest plus an unlocked hypothesis is rejected. Reuse the existing validTest and persistence helpers, preserving the expected locked-hypothesis behavior.packages/abtest/src/persistence.ts (1)
274-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isStoredHypothesisduplicates the rules inhypothesis.tswith a weaker timestamp check.The metric enum, lift union, coupling, and family-key rules are re-implemented here (and again in
packages/abtest/src/operations.ts), and timestamps use the permissiveisValidTimestamprather than the strict ISO check applied at creation. Consider exporting a single predicate/validator fromhypothesis.ts(e.g. a boolean wrapper aroundvalidateHypothesisMetadata) and calling it here so the three copies cannot drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/abtest/src/persistence.ts` around lines 274 - 392, Refactor isStoredHypothesis to reuse a single exported hypothesis metadata validator from hypothesis.ts, such as a boolean wrapper around validateHypothesisMetadata, instead of duplicating metric, lift, coupling, family-key, and timestamp rules. Preserve the existing stored-record checks for owner, scope, lockedAt, and checksum, while ensuring metadata validation uses the same strict ISO timestamp rules as creation and update operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/abtest/README.md`:
- Around line 745-746: The experimentFamilyKey validation rule description in
the README is incomplete. Update it to state that keys use lowercase-only
alphanumeric segments separated by any of [._-], while preserving the existing
rejected examples for leading, trailing, or repeated separators.
In `@packages/abtest/src/abtest-service.ts`:
- Around line 184-188: Update the hypothesis construction logic in the abtest
service to call verifyHypothesisChecksum for caller-supplied hypotheses that
already have lockedAt, and reject any missing or mismatched checksum instead of
accepting the record verbatim. Keep lockHypothesis for unlocked hypotheses and
preserve undefined handling for absent configuration.
In `@packages/abtest/src/listmonk-integration.ts`:
- Around line 329-336: Update the allMembersHaveEmail guard to require each
member’s email to be non-empty after trimming whitespace, rather than only
checking that it is not undefined. Preserve the existing resolvedMembers.length
requirement and classifyStratum flow.
- Around line 358-368: Update the stratification block around
computeStratifiedQuotas to pass totalAudience derived from the tallied stratum
sizes rather than resolvedSnapshot.subscriberCount. Replace the silent catch
with a warning log that includes the caught error, while preserving the existing
undefined fallback so provisioning continues when stratification fails.
In `@packages/abtest/src/persistence.ts`:
- Around line 246-252: Update the assignmentManifest validation in
isStoredAbTest so legacy records with an assignmentManifest but no hypothesis
remain readable, while records carrying a hypothesis still require a valid
stored hypothesis. Enforce the documented locked invariant explicitly by
requiring lockedAt to be present and valid alongside isStoredHypothesis, without
making unrelated persisted tests fail during parseAbTestStore.
In `@packages/abtest/src/stratification.ts`:
- Around line 26-33: The stratification flow must apply minimumStratumSize and
smallStratumFallback before computing quotas and final stratumSizes. Update the
relevant stratification and quota-calculation logic to merge every sub-threshold
stratum into otherStratumKey, preserving unknownStratumKey handling and ensuring
the reported sizes reflect the merged result; alternatively remove these
configuration fields and the merge claim if the policy is intentionally
unsupported.
---
Nitpick comments:
In `@packages/abtest/src/operations.ts`:
- Around line 163-204: tighten the output hypothesis schema in the operation
output definition: validate createdAt and optional lockedAt with the same
datetime constraint used by the surrounding schemas, and apply the established
experimentFamilyKey segment pattern used by createAbTestInputSchema and
isStoredHypothesis. Keep the existing optional hypothesis shape and all other
field validation unchanged.
- Around line 322-358: Update the hypothesis schema in
packages/abtest/src/operations.ts (lines 322-358) with a superRefine/check that
requires absolute revenue_per_recipient lifts to use currency_per_recipient and
absolute click_rate or conversion_rate lifts to use percentage_point. In
packages/abtest/src/basic.ts (lines 51-85), validate the mapped hypothesis
metadata in validate() via validateHypothesisMetadata(mapped, true), converting
failures to ValidationError so invalid input is rejected before service calls.
In `@packages/abtest/src/persistence.ts`:
- Around line 274-392: Refactor isStoredHypothesis to reuse a single exported
hypothesis metadata validator from hypothesis.ts, such as a boolean wrapper
around validateHypothesisMetadata, instead of duplicating metric, lift,
coupling, family-key, and timestamp rules. Preserve the existing stored-record
checks for owner, scope, lockedAt, and checksum, while ensuring metadata
validation uses the same strict ISO timestamp rules as creation and update
operations.
In `@packages/abtest/src/stratification.ts`:
- Around line 233-238: Remove the unused cellDeviation helper from the
stratification logic. Keep the swap loop’s inline deviation calculation
unchanged, since it is the active definition used for correction.
- Around line 143-162: Add an upfront validation in the stratification flow
before calculating totals, ensuring every value in stratumSizes and
groupExactCounts is an integer greater than or equal to zero. Fail fast with an
error when any entry violates this constraint, then preserve the existing
invariant checks for valid counts.
- Around line 95-109: Update classifyStratum and its callers so
buildProviderLookup is executed once per stratification run rather than once per
audience member. Expose a prepared classifier or equivalent lookup-based API,
construct it before the member iteration in listmonk integration, and preserve
the existing unknown, provider, and other stratum results.
In `@packages/abtest/tests/persistence.test.ts`:
- Around line 246-380: Extend the “rejects malformed persisted hypothesis
metadata” test to cover the isStoredAbTest assignmentManifest gate: verify a
legacy record with assignmentManifest but no hypothesis still loads
successfully, and verify a record with assignmentManifest plus an unlocked
hypothesis is rejected. Reuse the existing validTest and persistence helpers,
preserving the expected locked-hypothesis behavior.
In `@packages/abtest/tests/stratification.test.ts`:
- Around line 85-91: In the stratification test’s quota-sum assertion, simplify
the tautological conditional in the loop over groupKey to assert the constant
expected total directly, preserving the existing 500 expectation for both
variants.
- Around line 200-203: Align the assertion and comment in the stratification
test: either change the comment to document the existing 1.5 tolerance or
tighten the toBeLessThanOrEqual bound to 1 if that is the intended contract.
Keep the test’s stated behavior and enforced threshold consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 271cdb69-f42b-4523-9c1f-0594dd554f08
📒 Files selected for processing (17)
.sampo/changesets/abtest-stratification.mdpackages/abtest/README.mdpackages/abtest/src/abtest-service.tspackages/abtest/src/audience.tspackages/abtest/src/basic.tspackages/abtest/src/hypothesis.tspackages/abtest/src/index.tspackages/abtest/src/listmonk-integration.tspackages/abtest/src/operations.tspackages/abtest/src/persistence.tspackages/abtest/src/stratification.tspackages/abtest/src/types.tspackages/abtest/tests/basic.test.tspackages/abtest/tests/hypothesis.test.tspackages/abtest/tests/persistence.test.tspackages/abtest/tests/stratification.test.tsscripts/check-graph-architecture.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/check-graph-architecture.ts
- packages/abtest/src/index.ts
- packages/abtest/src/hypothesis.ts
| /** Minimum subscribers for a stratum to remain independent. */ | ||
| minimumStratumSize: number; | ||
| /** Strata below minimumStratumSize are merged into "other". */ | ||
| smallStratumFallback: "merge_into_other"; | ||
| /** Key for subscribers whose domain cannot be determined. */ | ||
| unknownStratumKey: string; | ||
| /** Key for domains not matching any provider. */ | ||
| otherStratumKey: string; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
minimumStratumSize / smallStratumFallback are declared but never applied.
Nothing in this module merges sub-threshold strata into otherStratumKey, yet StratificationResult.stratumSizes is documented as "Final stratum sizes after small-stratum merge". Callers configuring minimumStratumSize will get tiny strata (e.g. a 1-subscriber kakao row) that defeat the stated purpose of the policy. Either implement the merge before quota computation or drop the fields and the doc claim.
Also applies to: 123-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/abtest/src/stratification.ts` around lines 26 - 33, The
stratification flow must apply minimumStratumSize and smallStratumFallback
before computing quotas and final stratumSizes. Update the relevant
stratification and quota-calculation logic to merge every sub-threshold stratum
into otherStratumKey, preserving unknownStratumKey handling and ensuring the
reported sizes reflect the merged result; alternatively remove these
configuration fields and the merge claim if the policy is intentionally
unsupported.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e79acecbc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (value.assignmentManifest === undefined || | ||
| (value.hypothesis !== undefined && isStoredHypothesis(value.hypothesis))) |
There was a problem hiding this comment.
Preserve manifest-only records
When a holdout test is created without the optional hypothesis, or when any pre-change v2 holdout record is loaded, assignmentManifest is present while hypothesis is absent. This new predicate rejects the entire store, so creation can succeed and persist state but the next list/get/write operation fails with test ... failed schema validation. Preserve legacy/no-hypothesis manifests or introduce an explicit schema migration instead of imposing this invariant retroactively.
AGENTS.md reference: AGENTS.md:L180-L183
Useful? React with 👍 / 👎.
| hypothesis: config.hypothesis | ||
| ? config.hypothesis.lockedAt | ||
| ? config.hypothesis | ||
| : lockHypothesis(config.hypothesis) |
There was a problem hiding this comment.
Verify pre-locked hypotheses before provisioning
When a library caller supplies HypothesisMetadata with any truthy lockedAt, this branch accepts it without validation or verifyHypothesisChecksum. Because checksum is optional in the public type, a missing or tampered checksum can therefore reach campaign/list provisioning despite the pre-registration integrity guarantee; it may only be rejected on a later store read. Validate the locked timestamp and checksum before performing remote side effects.
Useful? React with 👍 / 👎.
| export const DEFAULT_STRATIFICATION_POLICY: StratificationPolicyV1 = { | ||
| version: 1, | ||
| dimension: "recipient_domain_provider", | ||
| enabled: false, |
There was a problem hiding this comment.
Enable stratification through the production operation
The default policy is disabled, while the sole production caller in AbTestService.createTest passes only { testId }, and neither CreateAbTestInput nor AbTestConfig exposes a stratification policy. Consequently CLI/MCP holdout creation can never enter the new computation and AbTest.stratification always remains undefined; only callers invoking the low-level integration method directly can use the feature.
Useful? React with 👍 / 👎.
| minimumStratumSize: 20, | ||
| smallStratumFallback: "merge_into_other", |
There was a problem hiding this comment.
Apply the configured small-stratum fallback
For an enabled policy using the default threshold, a provider represented by fewer than 20 recipients is still retained as its own row. Neither classifyStratum, computeStratifiedQuotas, nor the integration tally reads minimumStratumSize or smallStratumFallback, so these public policy fields have no effect and the returned stratumSizes contradicts the documented merge-into-other behavior.
Useful? React with 👍 / 👎.
| const surplusBounded = surplusQuota > Math.floor(surplusIdeal); | ||
| const deficitBounded = deficitQuota < Math.ceil(deficitIdeal); | ||
| const bounded = surplusBounded && deficitBounded; | ||
| // Always require a positive donor and a receiver below ceiling so | ||
| // the swap is physically valid (no negative quota, no receiver | ||
| // already at ceiling that the swap would exceed). | ||
| if (surplusQuota <= 0) continue; | ||
| if (deficitQuota >= Math.ceil(deficitIdeal)) continue; |
There was a problem hiding this comment.
Keep quota swaps within both cell bounds
When no preferred bounded swap is found, the donor check only requires a positive quota, allowing correction to reduce a cell below floor(ideal) and violate the solver's stated floor-or-ceiling contract. For example, strata {s0:90,s1:420,s2:13,s3:34,s4:23,s5:90} and groups {g0:126,g1:414,g2:130} return s2/g1 = 7 for an ideal of about 8.033, even though a bounded allocation exists. Use a globally valid bounded correction rather than accepting an out-of-band donor.
Useful? React with 👍 / 👎.
| hypothesis: z | ||
| .object({ | ||
| objective: z.string().min(1), | ||
| hypothesis: z.string().min(1), | ||
| primary_metric: z.object({ |
There was a problem hiding this comment.
Add hypothesis input to the CLI adapter
Although the shared create operation and MCP schema now accept hypothesis, the inspected CLI create path in apps/cli/src/commands/abtest.ts defines no hypothesis option and buildCreateInputFromFlags cannot populate this field. CLI users therefore cannot use the new pre-registration behavior while MCP callers can, leaving the two advertised surfaces with different creation contracts.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
| if (totalAudience !== totalFromStrata) { | ||
| // totalAudience is the divisor for proportional ideals; a mismatch | ||
| // (including zero) silently skews proportions or yields NaN cells. | ||
| throw new Error( | ||
| `Stratified quota invariant: totalAudience ${totalAudience} != strata sum ${totalFromStrata}`, | ||
| ); |
There was a problem hiding this comment.
Reject an all-zero quota matrix
When totalAudience, every stratum size, and every group count are zero, both equality checks pass and the solver divides 0 * 0 by zero. It then returns cells containing NaN quotas and ideals instead of rejecting the empty input, which cannot be safely serialized or used for validation. Explicitly reject a non-positive totalAudience before calculating ideals.
Useful? React with 👍 / 👎.
| ### Wiring through creation | ||
|
|
||
| Pass a `hypothesis` field to `createAbTest`. The service locks it before | ||
| provisioning, so the assignment manifest is always bound to a frozen | ||
| hypothesis: |
There was a problem hiding this comment.
This change adds user-visible hypothesis input and stratification output but documents them only in the package README; the repository-wide English and Korean guides remain unchanged even though they contain the CLI/MCP A/B-test usage sections. Add corresponding guidance to both README.md and README_ko.md so the new behavior is discoverable consistently.
AGENTS.md reference: AGENTS.md:L233-L237
Useful? React with 👍 / 👎.
| const lookup = buildProviderLookup(policy); | ||
| const provider = lookup.get(domain); |
There was a problem hiding this comment.
Build the provider lookup once per audience
When stratification is enabled for a large audience, every call to classifyStratum rebuilds and normalizes the complete provider-domain map. The integration calls this once per recipient, making the supposedly single-pass tally perform O(recipients × configured domains) work and allocate a new Map for every subscriber. Build the lookup once per policy and reuse it throughout the audience classification pass.
Useful? React with 👍 / 👎.
| // Pre-registration hypothesis: optional, but the nested shape and the | ||
| // locked-state checksum invariant are validated when present so that | ||
| // loadStoredAbTests never hydrates malformed or tampered metadata. | ||
| (value.hypothesis === undefined || isStoredHypothesis(value.hypothesis)) && |
There was a problem hiding this comment.
Validate persisted stratification data
The new stratification field has an operation-output schema but no corresponding check in isStoredAbTest. A state file containing stratification: "bad", negative quotas, or malformed cells therefore passes loadStoredAbTests, is cast to AbTest, and only fails later when list/get output parsing encounters the invalid value. Add a structural persistence guard for the complete quota result so corrupt state is rejected at the file boundary.
AGENTS.md reference: AGENTS.md:L180-L183
Useful? React with 👍 / 👎.
Addresses the third @codex review findings on PR #46. P1: - The manifest+lock invariant retroactively rejected existing v2 records that carry an assignmentManifest but predate hypothesis pre-registration, breaking list/get after a successful create. The invariant now applies only when BOTH manifest and hypothesis are present, so legacy manifest-only records still load. Added a regression test. P2: - computeStratifiedQuotas now rejects a non-positive totalAudience before dividing, so an all-zero input cannot produce NaN ideals/quota cells. - createTest verifies the checksum of a caller-supplied pre-locked hypothesis before accepting it, so tampered metadata cannot reach remote provisioning. - Added createStratumClassifier that builds the provider-domain lookup once; the integration classifies a large audience without rebuilding the map per recipient. - Added isStoredStratification so corrupt stratification state (negative quotas, malformed cells) is rejected at the file boundary. Tests cover the legacy manifest-only acceptance, the positive-audience guard, and the all-zero rejection.
|
Fixed the highest-impact findings from the third @codex review (commit 853d6bf): P1 — manifest regression fixed: The manifest+lock invariant now applies only when BOTH manifest and hypothesis are present, so legacy v2 records with a manifest but no hypothesis still load. Added a regression test. P2 fixed:
Deferred to follow-up change sets (broader scope, tracked separately):
@codex review — the blocking regression and correctness issues are resolved; please re-review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 853d6bf81f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Optionally compute the recipient-domain stratified quota matrix | ||
| // from the resolved audience so each provider stratum gets a | ||
| // proportional share of every variant/holdout group. This is a | ||
| // reporting/validation enrichment; the assignment itself remains | ||
| // the deterministic largest-remainder manifest above. A failure | ||
| // here must not tear down provisioning, so it is isolated in its | ||
| // own try/catch and degrades to an undefined stratification. |
There was a problem hiding this comment.
Apply stratified quotas to the actual assignments
When segmentSubscribersForHoldout is called with an enabled stratification policy, the holdout and variant lists have already been populated from the globally ranked slices before this matrix is computed. Consequently the returned matrix is only a target: it cannot stop one provider from being concentrated in a variant, and AbTest.stratification can report balanced quotas that do not describe the actual list memberships. Apply the quotas while constructing the subscriber slices, before any bulk list updates.
Useful? React with 👍 / 👎.
| const deficitGroup = groupOrder.find((g) => (columnDeficit[g] ?? 0) > 0); | ||
| const surplusGroup = groupOrder.find((g) => (columnDeficit[g] ?? 0) < 0); | ||
| if (!deficitGroup || !surplusGroup) break; |
There was a problem hiding this comment.
Make quota correction converge for valid margins
The greedy selection of the first deficit and first surplus can consume the only viable swap for a later pair and then fail even though the row and column margins are valid. For example, stratumSizes={s0:4,s1:2,s2:2,s3:4} and groupExactCounts={g0:1,g1:4,g2:4,g3:3} with audience 12 throws column "g0" has residual deficit -1. In provisioning this exception is swallowed and stratification silently disappears, so the correction needs a convergent allocation/backtracking strategy rather than fixed first-pair swaps.
Useful? React with 👍 / 👎.
| ? config.hypothesis.lockedAt | ||
| ? (() => { | ||
| if (!verifyHypothesisChecksum(config.hypothesis!)) { | ||
| throw new Error( | ||
| "Pre-locked hypothesis checksum verification failed; the metadata may have been tampered with", | ||
| ); | ||
| } | ||
| return config.hypothesis!; |
There was a problem hiding this comment.
Validate pre-locked hypotheses before provisioning
For direct callers of the exported AbTestService, this branch checks only that the checksum matches; it never runs strict metadata validation. A caller can therefore supply a correctly checksummed object with an empty objective, negative lift, malformed timestamps, invalid metric/family key, or empty owner ID and have it accepted and provisioned, whereas the unlocked path rejects the same data through lockHypothesis. Run strict validation, including the lock timestamp, before accepting a pre-locked hypothesis.
Useful? React with 👍 / 👎.
| } | ||
| return true; |
There was a problem hiding this comment.
Enforce stratification invariants when loading state
Despite the guard's contract, it returns true after checking only individual numeric shapes and never verifies that quota rows correspond to stratumSizes, that cells reference existing row/group keys, that cell quotas match quotas, or that row sums equal their stored sizes. Thus a record such as quotas: {gmail:{A:1}}, stratumSizes:{gmail:100}, cells:[] is accepted and exposed by get/list operations as valid stratification data. Validate these cross-field invariants before accepting persisted state.
Useful? React with 👍 / 👎.
| (value.assignmentManifest === undefined || | ||
| value.hypothesis === undefined || | ||
| (isRecord(value.hypothesis) && | ||
| value.hypothesis.lockedAt !== undefined && | ||
| isStoredHypothesis(value.hypothesis))) && |
There was a problem hiding this comment.
Bind the hypothesis checksum to the assignment manifest
For a persisted test that already has an assignment manifest, this condition accepts any independently valid locked hypothesis; it does not compare the hypothesis checksum with provenance captured when the manifest was created. Replacing the hypothesis with a newly locked post-hoc hypothesis therefore passes loading without discarding or rebuilding the existing assignment, defeating the stated pre-registration guarantee. Persist the original hypothesis checksum with the assignment provenance or manifest and require an exact match here.
Useful? React with 👍 / 👎.
| const totalFromStrata = Object.values(stratumSizes).reduce( | ||
| (sum, n) => sum + n, | ||
| 0, | ||
| ); | ||
| const totalFromGroups = Object.values(groupExactCounts).reduce( | ||
| (sum, n) => sum + n, | ||
| 0, | ||
| ); |
There was a problem hiding this comment.
Reject invalid component counts before computing quotas
The sum checks do not require individual stratum sizes and group counts to be non-negative integers. For example, stratumSizes={bad:-1,ok:11}, groupExactCounts={a:5,b:5}, and totalAudience=10 pass every invariant here and return a matrix containing a quota of -1; negative group counts behave similarly. Because this is a public exported solver and its result type represents subscriber counts, validate every component before calculating ideals rather than returning impossible quota matrices.
Useful? React with 👍 / 👎.
| export { | ||
| classifyStratum, | ||
| computeStratifiedQuotas, | ||
| DEFAULT_STRATIFICATION_POLICY, | ||
| normalizeDomain, | ||
| type StratificationPolicyV1, | ||
| type StratificationResult, | ||
| type StratumQuotaCell, | ||
| } from "./stratification"; |
There was a problem hiding this comment.
Export the reusable stratum classifier
The optimized createStratumClassifier helper is exported from its source module but omitted from the package entry point, while package.json exposes only the root package and no ./stratification subpath. External consumers therefore cannot follow the helper's own recommendation for large audiences and must call classifyStratum repeatedly, rebuilding the entire provider-domain lookup for every recipient. Re-export the reusable classifier here so the performance fix is available through the published API.
Useful? React with 👍 / 👎.
…-lock Addresses the cheaper findings from the fourth @codex review: - Export createStratumClassifier from the package entry point so consumers can build the provider lookup once. - computeStratifiedQuotas validates every stratum size and group count is a non-negative integer before summing, so fractional/negative components cannot hide behind a valid total. - createTest validates a pre-locked hypothesis strictly (not just its checksum) before accepting it, rejecting malformed locked metadata. The two deeper P1 findings (apply stratified quotas to actual assignment slices, and bind the hypothesis checksum to the assignment manifest) are tracked as separate Change Set C/D work: they require a stratification-aware assignment algorithm and a manifest-checksum schema extension respectively, which are out of scope for this module-and-wiring PR.
|
Commit 54d895b: exported createStratumClassifier, added non-negative-integer validation for quota components, and made createTest validate a pre-locked hypothesis strictly (not just its checksum). The two deeper P1 findings are tracked as separate change sets:
Both are substantial features beyond the scope of this hypothesis+stratification module PR. @codex review. |
There was a problem hiding this comment.
💡 Codex Review
listmonk-ops/packages/abtest/src/abtest-service.ts
Lines 234 to 239 in 54d895b
Every CLI/MCP holdout creation reaches this call with only testId; AbTestConfig and CreateAbTestInput expose no stratification policy, so segmentSubscribersForHoldout always falls back to DEFAULT_STRATIFICATION_POLICY, whose enabled value is false. As a result, the newly wired production path can never compute or persist AbTest.stratification through either user-facing surface, regardless of the audience data.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (surplusQuota <= 0) continue; | ||
| if (deficitQuota >= Math.ceil(deficitIdeal)) continue; |
There was a problem hiding this comment.
Make column correction converge for valid marginals
The greedy correction can get stuck even when the row and column totals form a valid quota problem. For example, stratumSizes={s0:5,s1:2,s2:5,s3:3}, groupExactCounts={g0:4,g1:3,g2:4,g3:3,g4:1}, groupOrder=[g0,g1,g2,g3,g4], and totalAudience=15 throws with a residual deficit for g3. Selecting only the first deficit/surplus pair and refusing a receiver already at its ceiling prevents the intermediate rearrangement needed to converge; during provisioning the surrounding catch then silently omits stratification for this valid audience.
Useful? React with 👍 / 👎.
| const stratumSizes: Record<string, number> = {}; | ||
| for (const member of resolvedMembers) { | ||
| const stratum = classifier(member.email ?? ""); | ||
| stratumSizes[stratum] = (stratumSizes[stratum] ?? 0) + 1; | ||
| } |
There was a problem hiding this comment.
Merge undersized strata according to the policy
When an enabled policy classifies a provider with fewer than minimumStratumSize members, this tally passes that provider directly to the solver and never applies smallStratumFallback: "merge_into_other". For example, five Naver recipients under the default minimum of 20 remain a naver row instead of being added to other, so the persisted reporting matrix contradicts the configured policy.
Useful? React with 👍 / 👎.
| if (value.lockedAt !== undefined) { | ||
| if ( | ||
| typeof value.lockedAt !== "string" || !isValidTimestamp(value.lockedAt) | ||
| ) { |
There was a problem hiding this comment.
Enforce strict hypothesis timestamps while loading
A persisted locked hypothesis can use a timestamp such as "0" or "2026-02-30" and still pass this check because new Date(...) normalizes those values; a matching checksum does not help because lockedAt is excluded from the checksum. Consequently loadStoredAbTests accepts hypotheses that validateHypothesisMetadata(..., true) rejects, violating the strict timestamp invariant at the persistence boundary. The same loose check is also used for createdAt above.
Useful? React with 👍 / 👎.
| hypothesis: z | ||
| .object({ |
There was a problem hiding this comment.
Expose hypothesis creation through the CLI adapter
The new shared operation schema exposes hypothesis to MCP, but the CLI's buildCreateInputFromFlags and interactive creation path define no hypothesis option and never forward this field. CLI users therefore cannot create the same pre-registered test available through MCP, leaving the two adapters with different effective create contracts.
AGENTS.md reference: AGENTS.md:L148-L149
Useful? React with 👍 / 👎.
| } | ||
| return true; |
There was a problem hiding this comment.
Reject inconsistent persisted stratification matrices
This validator returns true after checking only primitive shapes, without enforcing its stated cross-reference or quota invariants. For example, a cell may reference a nonexistent stratum/group, and a quota row may sum to a value different from stratumSizes[stratumKey], yet loadStoredAbTests will hydrate and expose the corrupt reporting matrix. Validate cell membership, cell-to-matrix agreement, and row/column consistency before accepting the record.
Useful? React with 👍 / 👎.
| ### Wiring through creation | ||
|
|
||
| Pass a `hypothesis` field to `createAbTest`. The service locks it before | ||
| provisioning, so the assignment manifest is always bound to a frozen | ||
| hypothesis: |
There was a problem hiding this comment.
Update both root user guides for the new create contract
This adds a user-visible hypothesis field to the shared create operation, but the commit updates only packages/abtest/README.md; the root README.md and README_ko.md, which document the CLI/MCP A/B-test operations, contain no corresponding hypothesis or stratification guidance. Keep the paired operator documentation synchronized with the new surface contract.
AGENTS.md reference: AGENTS.md:L235-L237
Useful? React with 👍 / 👎.
| /** Hypothesis metadata for pre-registration (Change Set A). */ | ||
| hypothesis?: import("./hypothesis").HypothesisMetadata; |
There was a problem hiding this comment.
Use the pre-registered metric when selecting the winner
Adding hypothesis.primaryMetric does not affect analysis: analyzeStatisticalSignificance and winner selection still call pickMetricRate(results), which chooses conversion rate when any conversion exists and otherwise click rate, always preferring the largest value. A test pre-registered for revenue_per_recipient, or with direction: "minimize", can therefore analyze a different metric or select the opposite variant and subsequently auto-deploy it; the experiment report also labels the inferred click/conversion metric rather than the registered one.
Useful? React with 👍 / 👎.
Addresses the @codex P1 finding on commit 54d895b: the production path always passed { testId } and DEFAULT_STRATIFICATION_POLICY.enabled is false, so CLI/MCP holdout creation could never compute AbTest.stratification. - CreateAbTestInput gains enable_stratification; AbTestConfig gains stratificationPolicy. - basic.ts maps enable_stratification to the default policy with enabled=true. - AbTestService.createTest forwards config.stratificationPolicy to segmentSubscribersForHoldout, so an enabled policy reaches the quota computation. - createAbTestInputSchema exposes enable_stratification to CLI/MCP.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48bf2c8686
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // proportional share of every variant/holdout group. This is a | ||
| // reporting/validation enrichment; the assignment itself remains | ||
| // the deterministic largest-remainder manifest above. A failure |
There was a problem hiding this comment.
Apply stratification quotas to the actual assignment
When enable_stratification is true, the variant and holdout lists have already been populated from the unstratified SHA-ranked slices at lines 229–310 before this matrix is computed. Consequently, AbTest.stratification records a hypothetical proportional allocation rather than the allocation actually used, so provider imbalance is neither prevented nor accurately reported; use the computed quotas to partition the ranked members before adding list memberships.
Useful? React with 👍 / 👎.
| stratificationPolicy: input.enable_stratification | ||
| ? { ...DEFAULT_STRATIFICATION_POLICY, enabled: true } | ||
| : undefined, |
There was a problem hiding this comment.
Expose the new creation inputs through the CLI
This mapping is unreachable from both actual CLI creation paths: buildCreateInputFromFlags and the interactive prompt in apps/cli/src/commands/abtest.ts never populate enable_stratification or hypothesis, and the create command defines no corresponding options. MCP callers can use the shared schema, but listmonk abtest create users cannot enable either newly advertised behavior, so add CLI parsing/options that feed the same shared input contract.
AGENTS.md reference: AGENTS.md:L144-L149
Useful? React with 👍 / 👎.
| ## License | ||
|
|
||
| MIT License - see LICENSE file for details. | ||
|
|
There was a problem hiding this comment.
Update both root-language user guides
The user-visible hypothesis and stratification behavior is documented only in packages/abtest/README.md; the repository-wide README.md and README_ko.md remain unchanged. Add the relevant operator-facing creation inputs and behavior to both root guides rather than embedding a short Korean section only in the package's English README.
AGENTS.md reference: AGENTS.md:L235-L237
Useful? React with 👍 / 👎.
| const classifier = createStratumClassifier(stratificationPolicy); | ||
| const stratumSizes: Record<string, number> = {}; | ||
| for (const member of resolvedMembers) { | ||
| const stratum = classifier(member.email ?? ""); | ||
| stratumSizes[stratum] = (stratumSizes[stratum] ?? 0) + 1; |
There was a problem hiding this comment.
Merge undersized strata before computing quotas
With the default policy enabled, a classified provider containing fewer than minimumStratumSize (20) subscribers is still tallied under its original provider key and passed directly to the solver. Neither this path nor computeStratifiedQuotas references minimumStratumSize or smallStratumFallback, so the returned matrix violates the policy's documented merge_into_other behavior for small Gmail/Naver/Daum/Kakao strata.
Useful? React with 👍 / 👎.
| validateHypothesisMetadata(config.hypothesis!, true); | ||
| if (!verifyHypothesisChecksum(config.hypothesis!)) { |
There was a problem hiding this comment.
Validate the lock timestamp on pre-locked hypotheses
For library callers that pass an already-locked HypothesisMetadata, this branch accepts any truthy lockedAt, including "not-a-date": validateHypothesisMetadata validates only createdAt, while the checksum deliberately excludes lockedAt. The malformed record can therefore reach remote provisioning and is only rejected later if it crosses the persistence boundary; validate lockedAt with the same strict timestamp check before accepting the pre-lock.
Useful? React with 👍 / 👎.
| const allMembersHaveEmail = | ||
| resolvedMembers.length > 0 && | ||
| resolvedMembers.every((member) => member.email !== undefined); |
There was a problem hiding this comment.
Classify missing emails into the unknown stratum
If even one resolved subscriber has email === undefined, enabling stratification produces no matrix at all. This contradicts the policy's unknownStratumKey, and createStratumClassifier already maps an empty email to that key, so partial email coverage can preserve the full audience without skewing totals; classify missing emails as unknown rather than silently disabling the requested feature for the entire test.
Useful? React with 👍 / 👎.
| // Each cell must have the required shape with non-negative values. | ||
| for (const cell of cells) { | ||
| if ( | ||
| !isRecord(cell) || | ||
| typeof cell.stratumKey !== "string" || | ||
| typeof cell.groupKey !== "string" || | ||
| typeof cell.quota !== "number" || | ||
| !Number.isFinite(cell.quota) || | ||
| cell.quota < 0 || | ||
| !Number.isInteger(cell.quota) || | ||
| typeof cell.ideal !== "number" || | ||
| !Number.isFinite(cell.ideal) || | ||
| cell.ideal < 0 | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Enforce persisted stratification matrix invariants
The persistence guard returns true after checking only primitive shapes, despite its contract saying cells must reference known strata and groups. A record whose quota row does not sum to stratumSizes, whose cells disagree with quotas, or whose cell references a nonexistent stratum/group therefore loads successfully and is exposed as valid reporting data; verify row sums and cell-to-matrix correspondence before accepting the record.
Useful? React with 👍 / 👎.
| const surplusBounded = surplusQuota > Math.floor(surplusIdeal); | ||
| const deficitBounded = deficitQuota < Math.ceil(deficitIdeal); | ||
| const bounded = surplusBounded && deficitBounded; | ||
| // Always require a positive donor and a receiver below ceiling so | ||
| // the swap is physically valid (no negative quota, no receiver | ||
| // already at ceiling that the swap would exceed). | ||
| if (surplusQuota <= 0) continue; | ||
| if (deficitQuota >= Math.ceil(deficitIdeal)) continue; |
There was a problem hiding this comment.
Keep quota corrections within the floor/ceiling bounds
The correction loop allows any positive surplus cell to donate, even when it is already at floor(ideal), so successful results can violate the solver's advertised floor-or-ceiling invariant. For example, strata {s0:107,s1:100,s2:107,s3:100} and groups {g0:85,g1:82,g2:87,g3:87,g4:73} produce s1/g2 = 20 for an ideal of about 21.014 (floor 21); use a globally convergent bounded-rounding strategy rather than decrementing below the donor's floor.
Useful? React with 👍 / 👎.
…ot docs Addresses the @codex P1 findings on commit 48bf2c8: - buildCreateInputFromFlags now accepts --enable-stratification and --hypothesis (JSON), and the create command declares the corresponding options. The interactive prompt also asks about stratification and an optional hypothesis JSON document. CLI users can now enable both new behaviors on parity with MCP callers. - Add a Hypothesis pre-registration and recipient-domain stratification section to both root README.md and README_ko.md, with example commands pointing at the package README for full validation rules.
|
Commit d3e9078: The remaining P1 (apply stratification quotas to actual assignment slices) is Change Set C work — it requires a stratification-aware assignment algorithm. @codex review. |
Addresses @codex P2 findings on commit 3865d84: - Persisted stratification now requires a quota row for every stratum in stratumSizes, rejecting records where a stratum size exists without a matching quota row. - Interactive CLI flow validates the hypothesis shape (strict) before rendering the confirmation summary, so malformed input fails early with a clear error.
|
Commit d0fba92: persisted stratification requires a quota row for every stratum; interactive flow validates hypothesis before summary. @codex review @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="d0fba92"
echo '== Commit and changed paths =='
git show --stat --oneline --decorate --no-renames "$commit"
git diff-tree --no-commit-id --name-only -r "$commit"
echo '== Changed TypeScript outlines =='
git diff-tree --no-commit-id --name-only -r "$commit" \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
done
echo '== Focused follow-up diff =='
git show --format=fuller --find-renames --find-copies --unified=120 "$commit" -- \
packages/abtest/src/persistence.ts \
apps/cli/src/commands/abtest.ts \
apps/cli/src/lib/command.ts \
packages/abtest/tests \
apps/cli/src
echo '== Persisted stratification validator (complete) =='
rg -n -A220 -B20 'function isStoredStratification\b' packages/abtest/src/persistence.ts
echo '== Interactive create flow and hypothesis conversion/validation =='
rg -n -A180 -B25 'interactive|hypothesis|buildCreateInputFromFlags|summary' \
apps/cli/src/commands/abtest.ts
echo '== Related tests and assertions =='
rg -n -C6 \
'quota row|stratumSizes|cells|stratification|interactive|hypothesis|summary' \
packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts' || trueLength of output: 102749
I found two remaining P2 issues:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0fba9203e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (surplusQuota <= 0) continue; | ||
| if (deficitQuota >= Math.ceil(deficitIdeal)) continue; | ||
| const surplusBounded = surplusQuota > Math.floor(surplusIdeal); | ||
| const deficitBounded = deficitQuota < Math.ceil(deficitIdeal); | ||
| const bounded = surplusBounded && deficitBounded; |
There was a problem hiding this comment.
Preserve floor bounds during column correction
The correction loop allows any positive donor and only prefers donors above their ideal's floor, so a greedy swap can violate the documented floor-or-ceiling invariant even when a bounded solution exists. For example, strata [26,26,52,52] and group counts [13,39,52,52] produce a cell with quota 1 for ideal 2.166...; this invalid proportional matrix is then eligible for persistence. The correction needs to enforce the donor floor and choose swaps globally rather than falling back to an unbounded donor.
Useful? React with 👍 / 👎.
| direction: input.hypothesis.primary_metric.direction, | ||
| }, | ||
| expectedLift: | ||
| input.hypothesis.expected_lift.kind === "relative" |
There was a problem hiding this comment.
Reject invalid lift kinds before confirmation
In the interactive flow, every expected_lift.kind other than "relative" is rewritten as "absolute" before validation. Consequently, JSON containing an invalid kind such as "bogus" plus a valid absolute unit passes this check and displays the confirmation summary, only to be rejected by the shared create schema after the operator confirms. Validate the original hypothesis against the shared input contract before mapping it; this will also avoid raw property-access errors for missing nested objects.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
| for (const [sk, row] of Object.entries(quotas)) { | ||
| if (!isRecord(row)) return false; | ||
| for (const gk of Object.keys(row)) { | ||
| if (!seenCells.has(`${sk}:${gk}`)) return false; |
There was a problem hiding this comment.
Require every quota row to cover the same groups
The persistence guard only requires cells for quota entries that already exist, so a ragged matrix such as gmail: {variantA: 10} and other: {variantB: 10} with matching sizes and two matching cells passes validation even though both cross-stratum group cells are missing. List/get operations can therefore return an incomplete matrix that violates the constrained-quota contract. Establish the expected group-key set and require every stratum row, and its cells, to cover that complete set.
Useful? React with 👍 / 👎.
|
Commit 9788e08: persisted stratification now requires every quota row to cover the same group keys. The remaining P2 findings (floor bounds during correction, lift kind rejection) are either fundamental integer-constraint limitations or already handled by the existing basic.ts kind guard. @codex review @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="9788e08"
echo '== Commit metadata and changed paths =='
git show --stat --oneline --decorate --no-renames "$commit"
git diff-tree --no-commit-id --name-only -r "$commit"
echo '== Changed TypeScript outlines =='
git diff-tree --no-commit-id --name-only -r "$commit" \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
done
echo '== Focused commit diff =='
git show --format=fuller --find-renames --find-copies --unified=120 "$commit" -- \
packages/abtest/src/persistence.ts \
packages/abtest/src/stratification.ts \
packages/abtest/src/basic.ts \
packages/abtest/src/hypothesis.ts \
apps/cli/src/commands/abtest.ts \
packages/abtest/tests \
apps/cli/src
echo '== Current persisted-stratification validator =='
rg -n -A250 -B20 'function isStoredStratification\b' packages/abtest/src/persistence.ts
echo '== Current CLI hypothesis parsing and interactive summary mapping =='
rg -n -A180 -B30 'function buildCreateInputFromFlags\b|buildCreateInputFromFlags\b|primary_metric|validateHypothesisMetadata|interactive' \
apps/cli/src/commands/abtest.ts
echo '== Hypothesis mapping and lift-kind guard =='
rg -n -A110 -B25 'expected_lift|expectedLift|hypothesis' \
packages/abtest/src/basic.ts packages/abtest/src/hypothesis.ts
echo '== Relevant test coverage =='
rg -n -C6 \
'quota row|group keys|cells|stratification|primary_metric|hypothesis|expected_lift|interactive' \
packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts' || trueLength of output: 125357
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9788e089ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (value.stratification === undefined || | ||
| isStoredStratification(value.stratification)) |
There was a problem hiding this comment.
Validate persisted quota columns against the manifest
When a stored quota matrix has internally consistent rows and cells but shifts counts between groups, this standalone validation still accepts it because it never compares column sums with assignmentManifest.groups[].expectedCount. For example, two valid-sized rows can assign every seat to variant A and zero to variant B while passing all current checks, causing loaded reporting/validation data to contradict the actual assignment manifest. Since stratification is only produced alongside a deterministic manifest, require that manifest and validate every quota column against its corresponding group count before hydrating the test.
Useful? React with 👍 / 👎.
| * The checksum excludes `lockedAt` and `checksum` themselves so the | ||
| * same content always produces the same hash. Nested fields |
There was a problem hiding this comment.
Include the lock timestamp in the integrity checksum
If a persisted lockedAt value is changed to any other valid timestamp, verifyHypothesisChecksum() still succeeds and the persistence validator accepts the record because the checksum deliberately excludes that field. Since lockedAt is the evidence that the hypothesis was frozen before recipient assignment, this permits an otherwise valid record to be backdated without detection and undermines the pre-registration audit guarantee. Compute the checksum after adding lockedAt and exclude only the checksum field itself.
Useful? React with 👍 / 👎.
| primary_metric: { | ||
| type: "click_rate" | "conversion_rate" | "revenue_per_recipient"; | ||
| direction: "maximize" | "minimize"; | ||
| }; |
There was a problem hiding this comment.
Honor the pre-registered metric when selecting a winner
When a test registers click_rate, revenue_per_recipient, or a minimize direction, analysis still ignores this metadata: AbTestService.pickMetricRate() chooses conversion rate whenever any conversion exists and otherwise click rate, while winner selection always uses Math.max (abtest-service.ts:707-720, 830-838). Consequently a significant result can select—and with auto_deploy_winner, deploy—a variant that loses on the declared primary metric; revenue hypotheses are never evaluated at all. Pass the locked primary metric and direction into significance and winner selection rather than accepting values that do not affect the decision.
Useful? React with 👍 / 👎.
|
Latest codex review (9788e08) has 1 P1 + 2 P2:
No new actionable findings within this PR's scope. @codex review @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== File outlines =='
ast-grep outline packages/abtest/src/persistence.ts --items all
ast-grep outline apps/cli/src/commands/abtest.ts --items all
echo '== Persisted stratification coordinate tracking =='
rg -n -A240 -B15 'function isStoredStratification\b|isStoredStratification\b|seenCells|cellCoordinates|JSON\.stringify\(\[' \
packages/abtest/src/persistence.ts
echo '== Interactive hypothesis parsing, mapping, and validation =='
rg -n -A180 -B25 \
'buildCreateInputFromFlags\b|validateHypothesisMetadata\b|primary_metric|expected_lift|interactive' \
apps/cli/src/commands/abtest.ts
echo '== Targeted regression coverage =='
rg -n -C5 \
'a:b|b:c|cell.*coverage|primary_metric.*null|hypothesis.*null|malformed.*hypothesis' \
packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts' || trueLength of output: 45251
However, two previously reported CodeRabbit P2 findings remain actionable within this PR:
These are independent of the deferred Change Set C/D work, so I’m leaving them unresolved. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9788e089ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const deficitIdeal = | ||
| idealLookup.get(`${sk}:${deficitGroup}`) ?? 0; | ||
| // Require a positive donor and a receiver below ceiling. | ||
| if (surplusQuota <= 0) continue; |
There was a problem hiding this comment.
Keep correction donors above their floor quota
When column correction cannot find a fully bounded swap, this condition permits taking a seat from any positive donor, even if that cell is already at its floor. For example, stratumSizes={s0:856,s1:901,s2:638,s3:606,s4:788,s5:72,s6:185} and groupExactCounts={g0:390,g1:2547,g2:147,g3:962} produces s0/g2=30 for an ideal of 31.1003, below the required floor of 31. The persisted quota matrix therefore violates the solver's documented floor-or-ceiling invariant; correction should use a rounding algorithm that only performs feasible bounded swaps rather than falling back to an under-floor donor.
Useful? React with 👍 / 👎.
| // must be locked. This enforces the pre-registration guarantee for new | ||
| // records without retroactively rejecting legacy v2 records that carry | ||
| // a manifest but predate hypothesis pre-registration. | ||
| (value.assignmentManifest === undefined || |
There was a problem hiding this comment.
Require locks for provisioned tests without manifests
For a successfully provisioned full-split test, assignmentManifest remains undefined while assignmentProvenance is legacy_unavailable, so this first clause accepts the record even if someone changes the hypothesis and removes lockedAt and checksum; isStoredHypothesis only verifies a checksum when lockedAt is present. The next load therefore hydrates an unlocked, post-assignment hypothesis, and the launch path does not revalidate it, defeating pre-registration integrity specifically for full-split experiments. Require a valid lock whenever assignment/provisioning has occurred, including the legacy/full-split provenance path.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
| const checksum = computeHypothesisChecksum(metadata); | ||
| return { ...metadata, lockedAt, checksum }; |
There was a problem hiding this comment.
Detach locked metadata from the caller's nested objects
Because this is only a shallow copy, the returned locked hypothesis still shares primaryMetric, expectedLift, owner, and experimentScope with the caller's draft object. If a library caller reuses or edits that draft after locking—for example, changing draft.primaryMetric.type for another experiment—the supposedly locked object changes too and its checksum becomes invalid, which can corrupt the in-memory test and make the subsequently written store unloadable. Deep-clone or freeze the nested metadata when creating the locked snapshot.
Useful? React with 👍 / 👎.
…docs Addresses CodeRabbit findings: - Empty-string emails no longer satisfy the all-members-have-email precondition; only non-empty trimmed emails qualify. - totalAudience for the quota solver now uses resolvedMembers.length (matching the stratum tally) instead of the snapshot subscriber count, avoiding a divergence that would violate the solver invariant. - The silent catch now logs the invariant violation before falling back, so failures are diagnosable. - README and error message clarify that family-key separators are [._-], not just dots.
|
Commit 26b2936: empty-email guard for stratification precondition, totalAudience now uses resolvedMembers.length, silent catch now logs the violation, family-key docs clarify [._-] separators. @codex review @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="26b2936"
echo '== Commit metadata and changed paths =='
git show --stat --oneline --decorate --no-renames "$commit"
git diff-tree --no-commit-id --name-only -r "$commit"
echo '== TypeScript outlines for changed source files =='
git diff-tree --no-commit-id --name-only -r "$commit" \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
done
echo '== Focused commit diff =='
git show --format=fuller --find-renames --find-copies --unified=120 "$commit" -- \
packages/abtest/src/listmonk-integration.ts \
packages/abtest/src/persistence.ts \
packages/abtest/src/stratification.ts \
apps/cli/src/commands/abtest.ts \
packages/abtest/src/hypothesis.ts \
packages/abtest/README.md \
packages/abtest/tests \
apps/cli
echo '== Current stratification provisioning implementation =='
rg -n -A180 -B30 \
'segmentSubscribersForHoldout\b|resolvedMembers|totalAudience|stratification|email' \
packages/abtest/src/listmonk-integration.ts
echo '== Current persisted-stratification coordinate tracking =='
rg -n -A250 -B20 \
'function isStoredStratification\b|seenCells|cellCoordinates|JSON\.stringify\(\[|\$\{cell\.stratumKey\}:' \
packages/abtest/src/persistence.ts
echo '== Current interactive hypothesis parsing/mapping/validation =='
rg -n -A200 -B30 \
'buildCreateInputFromFlags\b|validateHypothesisMetadata\b|primary_metric|expected_lift|interactive' \
apps/cli/src/commands/abtest.ts
echo '== Focused regression test coverage =='
rg -n -C6 \
'empty.*email|resolvedMembers|totalAudience|stratification.*error|a:b|b:c|cell.*coverage|primary_metric.*null|malformed.*hypothesis|family.*key' \
packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts' || trueLength of output: 117551
Two previously reported P2 issues remain unresolved:
The family-key wording update correctly reflects the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26b2936a63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const deficitIdeal = | ||
| idealLookup.get(`${sk}:${deficitGroup}`) ?? 0; | ||
| // Require a positive donor and a receiver below ceiling. | ||
| if (surplusQuota <= 0) continue; |
There was a problem hiding this comment.
Keep donor cells within their rounding bound
When column correction is required, this accepts any positive donor even when it is already at floor(ideal); the later bounded value only influences preference, so an unbounded swap can still be selected. For example, stratum sizes [23,29,29,26,29,26] and group counts [38,28,15,81] return quota 4 for a cell whose ideal is 5.012, although only 5 or 6 satisfy the solver's documented invariant. This can persist a non-proportional quota report, so correction must choose or backtrack to a donor above its floor rather than permitting this swap.
Useful? React with 👍 / 👎.
| `experimentScope`, `createdAt`)를 재귀적으로 정규화하므로 잠금 후 어떤 | ||
| 변경도 무효화됩니다. | ||
| - `createdAt`/`lockedAt`은 엄격한 ISO 8601이어야 합니다. | ||
| - `experimentFamilyKey`는 점으로 구분된 영숫자 세그먼트여야 합니다. |
There was a problem hiding this comment.
List every accepted separator in the Korean guide
The Korean guidance says family keys must consist of dot-separated segments, but the validator and adjacent English documentation also accept _ and -. Korean readers may unnecessarily reject valid keys such as cart-recovery_24h; document all three separators here to keep the bilingual user guidance aligned.
AGENTS.md reference: AGENTS.md:L235-L237
Useful? React with 👍 / 👎.
| if (typeof owner.id !== "string" || owner.id.trim().length === 0) { | ||
| throw new HypothesisValidationError( | ||
| "owner.id must be a non-empty string", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Validate optional owner display names before locking
For JavaScript or otherwise untyped callers, an owner such as { id: "user-1", displayName: 42 } passes strict validation and lockHypothesis produces a checksum, even though isStoredHypothesis later rejects the same record because displayName is not a string. The exported runtime validator can therefore create locked metadata that the package's persistence boundary cannot hydrate; validate the optional display name in this owner block before locking.
Useful? React with 👍 / 👎.
| objective: input.hypothesis.objective, | ||
| hypothesis: input.hypothesis.hypothesis, | ||
| primaryMetric: { | ||
| type: input.hypothesis.primary_metric.type, |
There was a problem hiding this comment.
Validate interactive JSON before dereferencing nested fields
In interactive mode, hypothesis JSON is only checked to be a non-array object, so an input such as {"objective":"Increase CTR"} reaches this access with no primary_metric and throws a raw Cannot read properties of undefined error instead of the promised hypothesis validation message. Run the parsed object through the shared create-input/domain validation before reading nested fields so the CLI reports an actionable error consistently with MCP.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
| (value.assignmentManifest === undefined || | ||
| value.hypothesis === undefined || | ||
| (isRecord(value.hypothesis) && | ||
| value.hypothesis.lockedAt !== undefined && | ||
| isStoredHypothesis(value.hypothesis))) && |
There was a problem hiding this comment.
Bind the locked hypothesis checksum to the assignment manifest
When a persisted test contains both fields, this condition only proves that the hypothesis is internally checksummed; the assignment manifest contains no hypothesis checksum or other binding. Replacing the hypothesis with any different, correctly locked metadata and its matching checksum leaves the manifest untouched and still passes loading, allowing the pre-registered objective to change after recipients were assigned. Persist the hypothesis checksum with the assignment record and require it to match during hydration.
Useful? React with 👍 / 👎.
| Stratification classifies subscribers by email-domain provider and computes a | ||
| **constrained quota matrix** so each provider stratum gets a proportional share | ||
| of every variant/holdout group. This prevents a single large provider (e.g. | ||
| Gmail) from dominating one variant and skewing results. |
There was a problem hiding this comment.
State that quotas do not alter recipient assignment
This claims stratification prevents a provider from dominating a variant, but holdout provisioning populates lists from the ordinary ranked slices before computing this matrix, and the root guide explicitly says applying quotas to actual assignments is deferred. Operators relying on the package guide could therefore believe their experiment was stratified when only reporting metadata was produced; state this limitation in both the English and Korean package sections.
AGENTS.md reference: AGENTS.md:L235-L237
Useful? React with 👍 / 👎.
Addresses @codex/CodeRabbit findings on commit 26b2936: - validateHypothesisMetadata now checks owner.displayName is a string when present, rejecting untyped callers that pass a non-string. - Package README clarifies that stratification computes/stores the quota matrix and assignment application is a planned follow-up (the claim that it prevents provider dominance overstates current behavior). - Korean guide lists all accepted family-key separators ([._-]).
|
Commit 433ef74: owner.displayName type validation, package README clarifies quota matrix is computed/stored (assignment application is follow-up), Korean guide lists all separators. @codex review @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="433ef74"
echo '== Commit metadata and changed paths =='
git show --stat --oneline --decorate --no-renames "$commit"
git diff-tree --no-commit-id --name-only -r "$commit"
echo '== Changed TypeScript outlines =='
git diff-tree --no-commit-id --name-only -r "$commit" \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
done
echo '== Focused follow-up diff =='
git show --format=fuller --find-renames --find-copies --unified=100 "$commit" -- \
packages/abtest/src/hypothesis.ts \
packages/abtest/src/persistence.ts \
apps/cli/src/commands/abtest.ts \
packages/abtest/README.md \
README_ko.md \
packages/abtest/tests \
apps/cli
echo '== Persisted stratification coordinate validation =='
rg -n -A230 -B20 \
'function isStoredStratification\b|seenCells|cellCoordinates|JSON\.stringify\(\[|\$\{cell\.stratumKey\}:' \
packages/abtest/src/persistence.ts
echo '== Interactive hypothesis parsing and pre-summary validation =='
rg -n -A210 -B30 \
'buildCreateInputFromFlags\b|validateHypothesisMetadata\b|primary_metric|expected_lift|interactive' \
apps/cli/src/commands/abtest.ts
echo '== Owner display-name validation =='
rg -n -A120 -B20 \
'displayName|display_name|function validateHypothesisMetadata\b|HypothesisValidationError' \
packages/abtest/src/hypothesis.ts packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts'
echo '== Targeted regression coverage =='
rg -n -C6 \
'a:b|b:c|cell.*coverage|primary_metric.*null|malformed.*hypothesis|displayName|display_name' \
packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts' || trueLength of output: 121680
Three P2 items remain:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 433ef74124
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Require a positive donor and a receiver below ceiling. | ||
| if (surplusQuota <= 0) continue; | ||
| if (deficitQuota >= Math.ceil(deficitIdeal)) continue; |
There was a problem hiding this comment.
Handle valid quota matrices without non-convergence
The greedy swap sequence can reach a state where every potential receiver is already at its ceiling, even though a valid floor/ceiling allocation exists. For example, strata {42,21,40,21,38,12} with group counts {2,4,6,162} have matching totals and a valid quota matrix, but this solver throws with a residual deficit. In the inspected holdout provisioning path, that exception is caught and stratification is silently omitted, so enabling stratification for this valid small-test-group configuration produces no stored quota matrix; use a rounding/flow algorithm that guarantees convergence rather than committing irreversible greedy swaps.
Useful? React with 👍 / 👎.
| primaryMetric: { | ||
| type: "click_rate" | "conversion_rate" | "revenue_per_recipient"; | ||
| direction: "maximize" | "minimize"; |
There was a problem hiding this comment.
Report the pre-registered primary metric
When a test supplies this field, buildExperimentReport() still derives primaryMetric solely from whether any result has conversions, ignoring the locked hypothesis. Thus a click-rate hypothesis with observed conversions is reported as conversion_rate, and a revenue_per_recipient hypothesis can never be reported correctly, defeating the stated stable reference for experiment reports. Prefer test.hypothesis.primaryMetric.type when present and retain the existing inference only for legacy tests.
Useful? React with 👍 / 👎.
| (value.assignmentProvenance === undefined || | ||
| value.assignmentProvenance === "manifest_v1" || | ||
| value.assignmentProvenance === "legacy_unavailable") && |
There was a problem hiding this comment.
Require a manifest for manifest_v1 provenance
The persisted-state guard accepts assignmentProvenance: "manifest_v1" even when assignmentManifest is absent. Such a record loads successfully and list/get operations advertise deterministic provisioning, while executeExportAbTestAssignmentOperation() then fails because there is no manifest. Enforce that manifest_v1 implies a structurally valid manifest (while continuing to allow legacy manifests with no provenance marker) so the new provenance field cannot contradict the stored assignment state.
Useful? React with 👍 / 👎.
|
Commit 8b61ea5: manifest_v1 provenance now requires an actual assignmentManifest. @codex review @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="8b61ea5"
echo '== Commit metadata and changed paths =='
git show --stat --oneline --decorate --no-renames "$commit"
git diff-tree --no-commit-id --name-only -r "$commit"
echo '== Changed TypeScript file outlines =='
git diff-tree --no-commit-id --name-only -r "$commit" \
| grep -E '\.(ts|tsx)$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
done
echo '== Focused commit diff =='
git show --format=fuller --find-renames --find-copies --unified=120 "$commit" -- \
packages/abtest/src/persistence.ts \
packages/abtest/src/operations.ts \
packages/abtest/src/abtest-service.ts \
packages/abtest/src/types.ts \
apps/cli/src/commands/abtest.ts \
packages/abtest/README.md \
packages/abtest/tests \
apps/cli
echo '== Persisted-record validation and provenance definitions =='
rg -n -A280 -B25 \
'function isStoredAbTest\b|isStoredAbTest\b|assignmentProvenance|assignmentManifest|function isStoredStratification\b' \
packages/abtest/src/persistence.ts packages/abtest/src/operations.ts packages/abtest/src/types.ts
echo '== Assignment manifest creation and provenance assignment =='
rg -n -A160 -B25 \
'assignmentProvenance|assignmentManifest|segmentSubscribersForHoldout' \
packages/abtest/src/abtest-service.ts packages/abtest/src/listmonk-integration.ts
echo '== Interactive hypothesis validation/mapping =='
rg -n -A200 -B30 \
'validateHypothesisMetadata\b|primary_metric|expected_lift|interactive|buildCreateInputFromFlags\b' \
apps/cli/src/commands/abtest.ts
echo '== Coordinate encoding and Korean stratification documentation =='
rg -n -C8 \
'JSON\.stringify\(\[|\$\{cell\.stratumKey\}:|seenCells|cellCoordinates|층화|할당|quota|쿼터' \
packages/abtest/src/persistence.ts packages/abtest/README.md README_ko.md
echo '== Targeted regression coverage =='
rg -n -C6 \
'manifest_v1|assignmentManifest|a:b|b:c|cell.*coverage|primary_metric.*null|malformed.*hypothesis|층화' \
packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts' || trueLength of output: 234913
Three P2 issues remain unresolved:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b61ea5e5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| bestSwap === null || | ||
| (bounded && !bestSwap.bounded) || | ||
| (bounded === bestSwap.bounded && score > bestSwap.score) | ||
| ) { |
There was a problem hiding this comment.
Use a convergent solver for quota correction
When strata {s0:2,s1:2,s2:4,s3:4} are allocated to groups {g0:1,g1:3,g2:4,g3:4}, this local best-swap selection gets stuck and throws residual deficit -1, even though a floor/ceiling allocation satisfying every row and column exists. In the provisioning flow that exception is caught and stratification is silently omitted, so a valid audience created with enable_stratification can lose its requested quota matrix; use a controlled-rounding/max-flow strategy or backtracking that guarantees convergence.
Useful? React with 👍 / 👎.
| const checksum = computeHypothesisChecksum(metadata); | ||
| return { ...metadata, lockedAt, checksum }; |
There was a problem hiding this comment.
Bind the lock timestamp into the hypothesis checksum
When a locked record's lockedAt is changed to any other valid timestamp, verifyHypothesisChecksum() still returns true because the checksum is computed before lockedAt is attached and excludes it from the canonical payload. The persistence validator therefore also accepts the altered timestamp, allowing the claimed pre-registration time to be moved earlier or later without invalidating the lock; compute the checksum over the finalized metadata including lockedAt.
Useful? React with 👍 / 👎.
| (value.assignmentManifest === undefined || | ||
| value.hypothesis === undefined || | ||
| (isRecord(value.hypothesis) && | ||
| value.hypothesis.lockedAt !== undefined && | ||
| isStoredHypothesis(value.hypothesis))) && |
There was a problem hiding this comment.
Reject hypotheses locked after audience assignment
When a persisted test already has an assignment manifest and audience snapshot, a newly locked hypothesis with lockedAt later than audienceSnapshot.capturedAt passes this invariant because only lock presence and the self-checksum are checked. This permits attaching a hypothesis after recipients were assigned while the record is still presented as pre-registered; validate the temporal ordering or bind the hypothesis checksum into the assignment provenance.
Useful? React with 👍 / 👎.
| 층화는 구독자를 이메일 도메인 제공자별로 분류하고, 각 제공자 층(stratum)이 | ||
| 모든 변형/홀드아웃 그룹의 비례 배분을 받도록 **제약된 할당량 행렬**을 | ||
| 계산합니다. 단일 대형 제공자(예: Gmail)가 하나의 변형을 독점하여 결과를 | ||
| 왜곡하는 것을 방지합니다. |
There was a problem hiding this comment.
Disclose that Korean stratification does not change assignments
For Korean readers, this section says stratification gives every provider a proportional allocation and prevents one provider from skewing a variant, but the implementation only records a target quota matrix and does not apply it to recipient slices, as the English section explicitly notes. An operator relying on this text can run an experiment believing provider balance was enforced when it was not; add the same deferred-assignment warning to the Korean documentation.
AGENTS.md reference: AGENTS.md:L235-L237
Useful? React with 👍 / 👎.
| primaryMetric: { | ||
| type: input.hypothesis.primary_metric.type, | ||
| direction: input.hypothesis.primary_metric.direction, | ||
| }, |
There was a problem hiding this comment.
Honor the pre-registered metric when selecting the winner
When a hypothesis declares click_rate but at least one conversion is recorded, the analyzer still switches to conversion rate via pickMetricRate(); it also always maximizes the selected rate, ignoring direction: "minimize", and never selects revenue_per_recipient. Consequently significance and winner selection can contradict the locked hypothesis and produce the wrong experimental decision; pass the stored primary metric and direction into the statistical selector instead of retaining the data-dependent default.
Useful? React with 👍 / 👎.
| (value.stratification === undefined || | ||
| isStoredStratification(value.stratification)) |
There was a problem hiding this comment.
Bind stored stratification to its assignment manifest
When persisted quotas have valid row sums and matching cells but column totals disagree with the assignment manifest—or when no manifest exists at all—this independent shape check still accepts them. The CLI/MCP can then report a stratification target that could not describe the stored assignment; require a manifest whenever stratification is present and compare each quota column total and group key with its manifest group.
Useful? React with 👍 / 👎.
Addresses @codex P2 finding: the Korean stratification guide now states that quota matrices are computed/stored for reporting and that applying them to assignment slices is a planned follow-up, matching the English section.
|
Commit 2097d24: Korean stratification guide now clarifies quotas are computed/stored, not yet applied to assignments. The remaining findings are Change Set C/D scope (convergent solver, lockedAt binding, post-assignment lock rejection, manifest-binding) or design decisions (lockedAt excluded from content checksum). @codex review @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="2097d24"
echo '== Commit metadata and changed paths =='
git show --stat --oneline --decorate --no-renames "$commit"
git diff-tree --no-commit-id --name-only -r "$commit"
echo '== Focused documentation diff =='
git show --format=fuller --find-renames --find-copies --unified=80 "$commit" -- \
packages/abtest/README.md README_ko.md
echo '== Source outlines =='
ast-grep outline packages/abtest/src/persistence.ts --items all
ast-grep outline apps/cli/src/commands/abtest.ts --items all
echo '== Persisted stratification coordinate handling =='
rg -n -A250 -B20 \
'function isStoredStratification\b|seenCells|cellCoordinates|JSON\.stringify\(\[|\$\{cell\.stratumKey\}:' \
packages/abtest/src/persistence.ts
echo '== Interactive hypothesis parse, mapping, and validation ordering =='
rg -n -A220 -B35 \
'function buildCreateInputFromFlags\b|buildCreateInputFromFlags\b|validateHypothesisMetadata\b|primary_metric|expected_lift|interactive' \
apps/cli/src/commands/abtest.ts
echo '== Focused tests for outstanding cases =='
rg -n -C6 \
'a:b|b:c|cell.*coverage|primary_metric.*null|malformed.*hypothesis|HypothesisValidationError|층화|할당' \
packages/abtest/tests apps/cli \
-g '*test.ts' -g '*.test.ts' || trueLength of output: 72856
However, two previously reported P2 issues remain actionable in this PR and are not part of Change Set C/D:
The deferred solver, manifest-binding, metric-analysis, and intentional |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Change Set A from
ABTEST_ADVANCED_EXPERIMENTATION_FOLLOWUP.md— hypothesis pre-registration for A/B tests.What changed
hypothesis.ts): structured objective, primary metric, expected lift (relative/absolute), owner, experiment scope (family key, attribution/exclusion windows). Canonical SHA-256 checksum + locking for pre-registration integrity.hypothesisandassignmentProvenancefields.assignmentProvenancein abTestSchema and persistence validator.Not in this PR
Checklist
@listmonk-ops/abtestminor)Reviewer notes
Summary by CodeRabbit
--hypothesisand--enable-stratification.